Skip to content

Emit error on array size overflow #4131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Emit error on array size overflow #4131

wants to merge 1 commit into from
+16 −0

Conversation

@P-E-P
Copy link
Member

@P-E-P P-E-P commented Sep 3, 2025

When the byte size required for an array overflow we should emit an error.

Fixes #3962

@P-E-P P-E-P force-pushed the huge_array_allocation branch from 8dbcf21 to 455cf4c Compare September 3, 2025 17:17
@P-E-P P-E-P marked this pull request as ready for review September 3, 2025 17:19
@P-E-P P-E-P requested a review from philberty September 3, 2025 17:20
@P-E-P P-E-P force-pushed the huge_array_allocation branch 2 times, most recently from f860507 to ad2b82c Compare September 3, 2025 17:34
philberty
philberty approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one didnt know you can do that

@philberty philberty force-pushed the huge_array_allocation branch from ad2b82c to c9af76e Compare September 3, 2025 19:23
philberty
philberty reviewed Sep 3, 2025
View reviewed changes
@philberty
Copy link
Member

philberty commented Sep 3, 2025

I think your Fix is partly right there are two parts to fix this properly.

  1. Overflow check the value
  2. Add a MAX alloc limit of 2gb i think is what rustc does from reading it ages ago.

So 1,

I think you need to add a check like you have or you could put it into typechecking over in

gccrs/gcc/rust/typecheck/rust-hir-type-check-expr.cc

Line 1153 in b7c9aaa

tree capacity_value

But maybe its enough to leave it wher you have it.

You should be able to do:

if (TREE_OVERFLOW_P (capacity_expr)) 
{
}

We track the capacity expr as part of the Array type now.

PArt 2: detect the HUGE array


tree len = capacity_expr
tree esize = TYPE_SIZE_UNIT (elt_type);     // bytes, INTEGER_CST
tree bytes = fold_build2 (MULT_EXPR, sizetype,
                          fold_convert (sizetype, len),
                          fold_convert (sizetype, esize));

if (TREE_OVERFLOW_P (bytes))

Or also put in a new constant for 2gb maybe make it a new option in lang.opts so it can be changed but it defaults to 2gb as the final check.

I think should do this.

philberty
philberty requested changes Sep 3, 2025
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions to try in the comments

@philberty
Copy link
Member

philberty commented Sep 3, 2025

Also debug_tree is super useful here

@P-E-P P-E-P force-pushed the huge_array_allocation branch from c9af76e to 573c26a Compare September 4, 2025 09:42
@philberty
Copy link
Member

philberty commented Sep 4, 2025

I think this is a good sign your getting:

Executing on host: /home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../gccrs -B/home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../  /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs  -m32  -fdiagnostics-plain-output   -frust-incomplete-and-experimental-compiler-do-not-use   -S -o issue-3962.s    (timeout = 300)
spawn -ignore SIGHUP /home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../gccrs -B/home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../ /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs -m32 -fdiagnostics-plain-output -frust-incomplete-and-experimental-compiler-do-not-use -S -o issue-3962.s
/home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs:2:19: error: left shift count >= width of type
compiler exited with status 1
FAIL: rust/compile/issue-3962.rs  at line 3 (test for errors, line 2)
FAIL: rust/compile/issue-3962.rs (test for excess errors)
Excess errors:
/home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs:2:19: error: left shift count >= width of type

@philberty
Copy link
Member

philberty commented Sep 4, 2025

not100% sure whats the best way to add tests for errors on m32 vs m64 here @dkm might know better

@P-E-P
Copy link
Member Author

P-E-P commented Sep 4, 2025 •
edited
Loading 

2. Add a MAX alloc limit of 2gb i think is what rustc does from reading it ages ago.

Or also put in a new constant for 2gb maybe make it a new option in lang.opts so it can be changed but it defaults to 2gb as the final check.

Are you sure about that ?

https://godbolt.org/z/Mr684qqsT

rustc does not throw an error message when the allocation is too big.

EDIT: It does since rustc 1.55 although I can't find anything about a 2Gb limit.

@P-E-P P-E-P force-pushed the huge_array_allocation branch from 573c26a to 4b81f3e Compare November 12, 2025 12:40
@P-E-P
Emit error on array size overflow
649afeb 
When the byte size required for an array overflow we should emit an
error.

gcc/rust/ChangeLog:

	* backend/rust-compile-expr.cc (CompileExpr::array_copied_expr): Check
	for overflow on array memory size and emit an error.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3962.rs: New test.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P P-E-P force-pushed the huge_array_allocation branch from 4b81f3e to 649afeb Compare November 12, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philberty philberty philberty requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@P-E-P P-E-P

Labels

codegen

Projects

libcore 1.49
Status: Todo

Milestone

HIR Pipeline for libcore 1.49 Complete

Development

Successfully merging this pull request may close these issues.

crash with shifts inside array length

2 participants

@P-E-P @philberty